feat(helm): add HStore deployment chart - #3132
Conversation
|
Ordering note: this chart depends on #3119 and should land after it.
No file overlap between the two, so there is nothing to rebase — it is purely a merge-order dependency. |
|
Blocking, and it comes from a change in #3119 rather than from this PR's own code. What changed. With What breaks here. This chart sets Before that change it did not fail — it came up as What to do. The wrapper should write
Two notes while you are in there: |
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. In addition to the documented auth.admin_pa gap, the current chart does not actually gate Store startup or rolling progress on PD quorum and permits disruption budgets below the PD majority. Three independent current-head review lanes converged on these findings.
| until [ "$( | ||
| ok=0 | ||
| for peer in ${HEALTH_PEERS}; do | ||
| if curl -fsS "http://${peer}/v1/health" >/dev/null 2>&1; then |
There was a problem hiding this comment.
/v1/health handler returns success unconditionally and does not check for a Raft leader or member catch-up. Two REST endpoints can therefore release every Store while PD has no usable quorum; the PD/Store readiness probes use the same process-liveness signal, so a StatefulSet rolling update can also advance before the restarted member has rejoined safely. Please gate on an actual leader/quorum/catch-up-aware signal (for example, validated membership state) and add no-leader plus rolling-restart scenarios.
There was a problem hiding this comment.
Agreed on both counts: /v1/health is process liveness, not quorum, and the probes share the same weakness during rolling updates. The fix I intend is to gate the Store init container and the PD readiness signal on /v1/members (leader present, member state validated) rather than counting healthy responders, and to add the no-leader and PD rolling-restart scenarios you list to the full lifecycle matrix rerun already committed before this PR leaves draft. Landing it together with that rerun keeps the gating change and its runtime evidence in one reviewable step rather than shipping an untested probe contract now.
| templates require replicas > 1, so a single-replica release never creates one | ||
| and must not be failed for a value that has no effect. | ||
| */}} | ||
| {{- if and .Values.pd.pdb.enabled (gt (int .Values.pd.replicas) 1) (ge (int .Values.pd.pdb.minAvailable) (int .Values.pd.replicas)) -}} |
There was a problem hiding this comment.
pd.replicas=5 with minAvailable=2 renders successfully and permits three voluntary evictions, leaving 2/5 members and no quorum. When the PD PDB is enabled, require minAvailable >= floor(replicas / 2) + 1 (and define the supported even/odd replica contract), with boundary render tests for several replica counts.
There was a problem hiding this comment.
Fixed in ad52e70. When the PD PDB is enabled, render now requires minAvailable >= floor(replicas/2) + 1 in addition to the existing minAvailable < replicas bound, so pd.replicas=5 with minAvailable=2 is rejected. Boundary cases are in the CI invalid-value step: 5/2 and 4/2 fail, 5/3, 5/4, and 3/2 render. The even/odd contract is documented in the README: with 2 replicas the majority is the whole membership, so no valid budget exists and the PDB must be disabled or the replica count made odd.
| value: {{ include "hugegraph.store.restPrimary" . | quote }} | ||
| - name: HG_SERVER_INIT_STORE_ENABLED | ||
| value: {{ .Values.server.initStoreEnabled | quote }} | ||
| {{- with .Values.server.extraEnv }}{{ toYaml . | nindent 12 }}{{- end }} |
There was a problem hiding this comment.
extraEnv is appended after chart-owned variables, while the schema allows duplicate names. A user can therefore add another HG_SERVER_INIT_STORE_ENABLED=true and bypass the chart's validated false contract, re-enabling concurrent init-store across Server replicas; the PD and Store templates expose the same override path for their identity/topology variables. Please reject chart-reserved names in each component's extraEnv (or render managed variables last with an explicit precedence contract) and add negative render tests.
There was a problem hiding this comment.
Fixed in ad52e70. Each component's extraEnv now rejects its chart-managed variable names at render time (hugegraph.validateValues), covering the Server contract variables including HG_SERVER_INIT_STORE_ENABLED and PASSWORD, the PD and Store identity and topology variables, the Hubble wiring variables, and JAVA_OPTS. Rejection was chosen over reordering because rendering managed variables last would silently ignore the user's entry instead of failing it. Negative render cases for Server, PD, and Store overrides are in the CI invalid-value step, and a non-reserved name still renders.
…tore disabled The Server wrapper now writes auth.admin_pa from the auth Secret alongside usePD and pd.peers, so an auth-enabled release keeps its configured admin password with init_store.enabled=false instead of silently falling back to the public default. The Secret value is rejected when it contains properties-parser metacharacters that would inject config lines or store a different password than the Secret holds. The new hubble component deploys the Hubble UI as a single-replica Deployment with pd and direct wiring modes, optional Ingress and H2 persistence, schema validation, render-time guards, docs, and CI coverage. PD-meta installs (auth enabled, or Hubble in pd mode) also announce the Server client Service URL to PD via server.urls_to_pd and server.deploy_in_k8s so discovery clients receive a resolvable address instead of the 0.0.0.0 default, and the Hubble wrapper writes server.host so current images bind all interfaces. Because current Hubble images authenticate their login against the cluster, rendering Hubble without server.auth fails unless explicitly overridden. The CI invalid-value step now fails on every case rather than only its last line, and positive renders cover both Hubble modes. Validated against a composition of master 1716c77 plus the current heads of apache#3119 (edf07d0), apache#3126 (b40c42f), and apache#3130 (198de19): fresh auth-enabled installs reach Ready with zero restarts, the admin credential comes from the Secret while unauthenticated and default-password requests get 401, and Hubble logs in with the Secret credential and reads cluster metadata through PD discovery, with its H2 metadata persisted on the PVC.
|
Addressed in 34234e3. The wrapper now writes |
…xtraEnv overrides A PD PodDisruptionBudget with minAvailable below floor(replicas/2)+1 permits voluntary evictions that leave PD without a Raft majority, so the render now requires the quorum floor in addition to the existing blocks-all-drains upper bound. With 2 replicas no valid budget exists; the README documents the even/odd contract. extraEnv entries render after the chart-owned variables and Kubernetes lets the last duplicate win, so a duplicate name could silently override a validated contract such as HG_SERVER_INIT_STORE_ENABLED=false. Each component's extraEnv now rejects its chart-managed variable names. Boundary and negative render cases for both rules are part of the CI invalid-value step.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #3132 +/- ##
============================================
- Coverage 39.19% 1.54% -37.65%
+ Complexity 264 21 -243
============================================
Files 770 748 -22
Lines 65779 63264 -2515
Branches 8726 8278 -448
============================================
- Hits 25779 975 -24804
- Misses 37247 62205 +24958
+ Partials 2753 84 -2669 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The run failed at startup because azure/setup-helm@v4 is not on the ASF-approved actions allowlist; install the pinned helm release from the official tarball in a plain run step instead.
…cheduling defaults Fresh installs now seed PD with partition.default-shard-count derived from the Store count (3 when store.replicas is at least 3, otherwise 1, matching PD's odd-only constraint and its 2-to-1 clamp), so a default 3-Store deployment gets store-level HA instead of the image default of one shard replica per partition. The seed is delivered as -D system properties prepended into the PD JAVA_OPTS ahead of pd.javaOpts, preserving the start script's automatic heap sizing; it applies at first bootstrap only, after which PD metadata is authoritative, all documented together with the resulting initial partition count change. pd.antiAffinity and store.antiAffinity default to preferred so the chart schedules on clusters with fewer nodes than replicas; values-cluster.yaml keeps required for both, NOTES warns when PD quorum members may co-locate, and the README documents the upgrade implications. The Disaster Recovery documentation describes what current PD builds actually do: the scheduled patrol only marks silent stores Offline, shard reconciliation and tombstone processing run only via the manual /v1/task/patrolPartitions endpoint, and the pd.patrol-interval and store.max-down-time properties are bound but never read, which is why the chart does not expose them. Periodic leader balancing and recovery metrics are referenced as upstream feature requests. extraEnv now also rejects JAVA_OPTIONS for pd, store, and server, because the start scripts drop the chart-managed JAVA_OPTS entirely when it is set. Schema accepts numeric strings for the new keys, values-file integers at or above one million no longer fail as scientific notation, and CI asserts the rendered -D content, covers both shard-count validation messages, and kubeconforms the sharded render.
|
0ebd2dc addresses the deployment-side feedback from testing:
|
2 participants
Purpose of the PR
Add an official Helm installation path for the distributed HugeGraph HStore
topology on Kubernetes. The chart packages the PD, Store, and Server startup
contract that otherwise has to be reconstructed by each operator.
This PR stays a draft until its remaining prerequisite PRs land. The
previously open item under "Known gaps before this leaves draft" (the
auth.admin_pabootstrap gap) is now fixed and retested; see that sectionand the validation update below.
Main Changes
helm/hugegraphchart with chart version0.1.0:helm testconnection hook.values.schema.jsonwith render-time validation for replica counts,storage, PDBs, HPA, authentication, Services, and other value contracts.
disabled by default, configurable pod security contexts, hardened container
security defaults, graceful termination periods, probes, and
extraEnv.latestPD, Store, and Server images while this PR is a draft,using pull policy
Alwaysso nodes do not retain an older cached image.kubeconform, legacy-values, andpackaging CI.
limitations documentation, plus a root README installation link.
Recreatestrategy, ClusterIP Service, optional Ingress and persistence,hubble.mode(pdfor PD discovery and the operations view,directforthe Server Service only), probes, schema validation, render-time guards,
documentation, and CI coverage. Current Hubble images authenticate their
login against the cluster, so rendering Hubble without
server.authfailsunless explicitly overridden.
auth.admin_pafrom the auth Secret alongsideusePDandpd.peers, andrejects Secret values containing properties-parser metacharacters.
installs (auth enabled, or Hubble in
pdmode) writeserver.urls_to_pd(the Server client Service URL) and
server.deploy_in_k8s=true, replacingthe unusable
0.0.0.0:8080default that PD would otherwise hand toclients such as Hubble.
server.host=0.0.0.0because current Hubbleimages bind
server.host(defaultlocalhost) and ignore the legacyhubble.hostkey in the shipped configuration.rather than only the last line, and add positive renders for both Hubble
modes.
partition.default-shard-countderived from the Store count (3 whenstore.replicasis at least 3, otherwise 1, matching PD's odd-onlyconstraint), replacing the image default of one shard replica per
partition. The seed applies at first bootstrap only; PD metadata is
authoritative afterwards, and the resulting initial partition count
change is documented.
pd.antiAffinityandstore.antiAffinitytopreferredso thechart schedules on clusters with fewer nodes than replicas;
values-cluster.yamlkeepsrequiredand NOTES warns when PD quorummembers may co-locate.
actually do (manual
/v1/task/patrolPartitionsreconciliation, noautomatic re-replication), and reject
JAVA_OPTIONSinextraEnvforpd, store, and server because the start scripts drop the chart-managed
JAVA_OPTSwhen it is set.Distributed startup and recovery contracts
HG_SERVER_SKIP_INIT. It setsHG_SERVER_INIT_STORE_ENABLED=false, relying on the dedicatedinit_store.enabledgate from fix(dist): gate init-store on a dedicated init_store.enabled option #3119 (issue InitStore should be able to skip local init in PD/HStore deployments #3118) so concurrent Serverreplicas do not initialize the same distributed backend.
usePD=true,pd.peers,server.urls_to_pd,and
server.deploy_in_k8son PD-meta installs, and, when authenticationis enabled, writes
auth.admin_pafrom the mounted Secret before handingcontrol back to the image entrypoint.
auth.admin_paapplies only atfirst creation, so it does not rotate an existing cluster's password, and
it lands in
rest-server.properties(mode 600) inside the container.truncation, keeping names valid and PVC identities stable across scaling.
startup command.
Prerequisites
Three prerequisites have merged since this PR was opened:
b026a90a8b2932c7lsof09dbc744Three remain open, and this PR should land after all of them:
init_store.enabledgateedf07d0fb40c42fb198de19e#3126's finite DNS TTL is required for #3130 to resolve a replacement Store
address. Neither change provides complete address recovery alone.
#3119 has since been narrowed to the
init_store.enabledoption, its gate, theenv mapping, and the init-flag guard. The Docker entrypoint and auth-bootstrap
work that previously shared its branch is now tracked separately in #3133. That
split does not change what this chart needs from #3119, but it does mean the
entrypoint's
PASSWORDandauth.admin_pahandling is a known-imperfectcontract this chart depends on.
Validation baseline and head drift
The runtime matrix below was executed against a composition built from these
exact heads:
ee8f555909dbc7443e505a81ff1325ac(scope narrowed)35e1a2404d0ff03e(scope widened)0118e158575a112261407213, one commit further26218cb3ddeef7a9Only #3128 merged at exactly the head this chart was validated against. The
results below therefore describe the chart's behavior against a composition
that no longer matches upstream, and are reported as the current evidence
baseline rather than as a claim about today's
master.The
auth.admin_pafix and the Hubble component were validated separatelyagainst a newer composition:
masterat1716c774(which already carries#3105, #3128, and #3129) merged with #3119 at
edf07d0f, #3126 atb40c42fb, and #3130 at198de19e(the current heads of all three openprerequisites at the time of this update). Evidence from that composition:
Ready with zero restarts and
init_store.enabled=falsethroughout (nofail-closed refusal).
admin:paboth get401; the Secret credential gets 200.
and succeeds with the Secret credential in both
pdanddirectmodes;an authenticated session reads
/graphspacesfrom the cluster through PDdiscovery; with persistence enabled, the H2 metadata lands on the PVC.
pdmode confirmed the PD-metaproperties are written and discovery reaches the Server; it also showed
current Hubble images cannot complete their login against an auth-less
cluster (the server rejects
/auth/loginwith "Unconfiguredauthenticator"), which the chart now surfaces as a render-time guard.
issues (both fixed and re-verified): the CI invalid-value step enforced
only its last line, and the PD-meta properties were scoped to unrelated
settings.
The full 3 PD + 3 Store + 3 Server lifecycle matrix rerun is still pending
before this PR leaves draft, as committed below.
Known gaps before this leaves draft
auth.admin_pa. The gap flagged inreview (the chart set
HG_SERVER_INIT_STORE_ENABLED=falseand mountedPASSWORD, but never wroteauth.admin_pa, so the Secret was silentlyignored and, after fix(dist): gate init-store on a dedicated init_store.enabled option #3119, an auth-enabled deployment failed at container
start) is fixed: the wrapper writes
auth.admin_pafrom the same Secretalongside
usePDandpd.peers, rejecting values withproperties-parser metacharacters.
auth.admin_paapplies only at firstcreation, so it does not rotate an existing cluster's password, and it
lands in a mode-600 file inside the container; both caveats are in the
chart README.
Before this PR leaves draft I will fetch the current
master, rebase thechart, pin
appVersionand all component image tags (including Hubble) tothe next release, switch their pull policies to
IfNotPresent, verify themerged prerequisite behavior, rerun the affected full lifecycle matrix
against a composition of the then-current heads, and re-review any
resulting changes.
Verifying these changes
PASS=239 FAIL=0 SKIP=0.PASS=37 FAIL=0.--reuse-values:helm templateandhelm install --dry-run=clientpassed.61/0, dataset30/0.61/0, dataset30/0.PASS=91 FAIL=0.PASS=90 FAIL=0with data preserved.PASS=7 FAIL=0, with restoration verified.61/0and30/0.findings on the final template and schema implementation.
no templates or schema and repeated the full static gate at
239/0/0.The runtime matrix used a four-node Kubernetes cluster and images built from a
composition containing the exact prerequisite heads listed under "Validation
baseline and head drift". Test-only validation harnesses and lab-specific
low-memory values are excluded from this PR. As noted above, five of those six
heads have since moved, so this matrix will be rerun before the PR is marked
ready.
Does this PR potentially affect the following parts?
Documentation Status
Doc - TODODoc - DoneDoc - No NeedDocumented operational limits
appVersionand all three component image tags arelatest. This is intentionally temporary and is not the stable publicationconfiguration; the next release tag will be pinned before the PR is marked
ready.
values-cluster.yamlis a production starting point, not a capacityguarantee.
preferred, so the chart scheduleson any node count; production should pin
required(asvalues-cluster.yamldoes) so one node failure cannot take out the PDquorum or co-locate shard replicas.
applied from the Secret only at first creation; the chart cannot rotate an
existing cluster's admin password.
and its login requires
server.authon current images (the chart refusesto render it without auth unless explicitly overridden). Its UI connection
metadata, including graph credentials entered in the UI, lives in an
embedded H2 database that persists only when
hubble.persistenceisenabled; the PVC is kept on uninstall.
setting
runAsNonRootorreadOnlyRootFilesystem, which the current imagecontract cannot support.
tracked in [Bug] Docker entrypoint auth bootstrap is unsafe for mounted and upgraded configs #3133 and is outside this chart's control.
leader transfer, ConfigMap-based component configuration, and a complete
monitoring stack are outside this PR.
work, tracked in #3135;
the chart documents the manual balance endpoints as the current workaround.
work, tracked in #3136.
available on every Server replica) is documented as a chart limitation;
the underlying fix is upstream work, tracked in
#3137.
waits for its own graph binding before returning 200) is up in
#3138; the documented
chart-side mitigations remain useful until the rest of it lands.